-
-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add forked from feature UI #3371
Conversation
if let forkedFromURL { | ||
let repoURLNode: Node<HTML.BodyContext> = .a( | ||
.href(forkedFromURL), | ||
.text("repository") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complete outsider to conversations, so if this is the agreed solution then just ignore this.
It would feel a bit nicer to me if we were able to display the original author name here perhaps? "Forked from Apple" as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You took my words 😂
In discussions we talked about having it as "Forked from owner/repo.", which I think would be fine.
If we wanted to really push the boat out and be really extra, I could see this:
- If the original repo isn't on SPI, it says "Forked from owner/repo." and links to GitHub.
- If the original repo is on SPI and the package name (not repo name) is the same, it says "Forked from Original Owner Name" using the owner name that we already have. Thanks James for this suggestion.
- If, in addition to that, the package name has changed, it says "Forked from Original Package Name by Original Owner Name"
That'd be the absolute belt and braces implementation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go for the absolute belt and braces, I'll update the PR to add those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these changes! This is really close now. Just a couple of tweaks:
- In the case where we have a renamed package, the link looks good but let’s split the link into two so there’s a link to the package first, then the word “by” which isn’t a link (this will help visually distinguish the package name from the owner name), then a link to the owner’s page. Like this:
- I think it looks slightly odd where the package is not renamed. In some instances it's fine, but examples like XibLoc look odd and it’s not immediately obvious what “happn” is. Let’s slightly adjust this (and make it simpler) by including both the package name and the owner name in both cases, even if the package has not been renamed.
So. This:
Becomes this:
As mentioned above, this should also separate the one link into two. The first being a link to the package and the second being a link to the author. The good news is that we’ve also just reduced the number of possible code paths from three to two.
- Finally, I think there’s a crossed wire with the links to GitHub. When I said “Forked from owner/repository” I meant to replace the “repository” link with a link that says the owner and the repository variables, not the words.
So, for example. This:
Becomes this:
let ownerName = model?.repository.ownerName, | ||
let owner = model?.repository.owner, | ||
let packageName = model?.version.packageName else { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daveverwer @finestructure not entirely sure about this one. I saw that in some cases if the packageName
is null we then use the repo name instead of it. Should we go with the same approach here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, falling back to repositori name is the approach we usually use. I'll review this properly shortly.
36717e8
to
168f8ff
Compare
168f8ff
to
e286cae
Compare
FrontEnd/styles/package.scss
Outdated
@@ -69,12 +69,17 @@ | |||
display: block; | |||
font-size: 11px; | |||
} | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -40,4 +40,12 @@ extension Joined3 where M == Package, R1 == Repository, R2 == Version { | |||
.filter(Repository.self, \.$owner, .custom("ilike"), owner) | |||
.filter(Repository.self, \.$name, .custom("ilike"), repository) | |||
} | |||
|
|||
static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take @finestructure's view on this as he's the keeper of the Joined
classes, but I wonder if this might need a more descriptive name. It makes sense to query with the packageID, but the fact that this is filtering to the default branch version too is a little hidden from whoever is calling this function.
Alternatively, we could move the filter to outside this query and let the calling code add that filter.
Don't make any changes based on what I say to this, though as I know @finestructure will have a view on it and we should go with his recommendation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Dave, we shouldn't be filtering inside the method. If you look above, there's a version related overload and I'd do the same here. Keep the parameter, that way it's obvious at the call site that versions are filtered:
static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> { | |
static func query(on database: Database, | |
packageId: Package.Id, | |
version: Version.Kind) -> JoinedQueryBuilder<Self> { | |
query(on: database, version: version) | |
.filter(Package.self, \Package.$id == packageId) | |
} |
Call site:
let model = try await Joined3<Package, Repository, Version>
.query(on: database, packageId: packageId, version: .defaultBranch)
.first()
import Vapor | ||
|
||
extension API.PackageController { | ||
enum ForkedFromResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ForkedFromResult
and ForkedFromInfo
seem very similar. They have the same cases, almost the same associated values - could they be the same type? Is there a reason they're not? If so, perhaps the name could/should carry that concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are very similar. The reason I have two types is because I thought that it might be a good thing to separate the actual result and then to initialize the model (that's gonna be used in the view layer). Same thing if you look at PackageResult
and API.PackageController.GetRoute.Model
. I'm not really sure. Should I just merge it into one type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PackageResult
(a db model reference type) and API.PackageController.GetRoute.Model
(a view model value type) are great examples of why and when to add these types.
We could have avoided transforming data into the various types that make up GetRoute.Model
but we wanted to have a plain value type view model that we can use to drive a page. That's a clearer separation but it also allows us to very easily assemble data for snapshot testing etc.
(It was also really helpful early on when Dave designed the pages and just specified in a struct what data he needed and I populated it from the db results independently. It made for a great interface.)
You'll see that some types don't get translated that way. For example we pass App.Reference
and others 1:1 to GetRoute.Model
- and that's because they're plain value types already. They're easy to construct for the tests and equally simple to pass around.
ForkedFromXXX
seems to be the same: it's a value type and it really seems to be packaging all the same data. So it can just be the same type - we wouldn't need to transform it unless there's some other reason for it.
One such reason might be some computation that's happening. I can't think of a great example right now but we'd want to avoid having GetRoute.Model
do a lot other than just package up data for a page.
Does that make sense?
I can take a closer look myself to see if the types can be collapsed if you prefer, just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference FrokedFromInfo
has from ForkedFromResult
is this
var url: String {
switch self {
case .fromSPI(_, let originalOwner, _, let originalRepo, _):
return SiteURL.package(.value(originalOwner), .value(originalRepo), nil).relativeURL()
case .fromGitHub(let url):
return url
}
}
var ownerURL: String? {
switch self {
case .fromSPI(_, let owner, _, _, _):
return SiteURL.author(.value(owner)).relativeURL()
case .fromGitHub:
return nil
}
}
it has some computed variables which come in handy in the view itself - so I think they are totally mergeable. But where should the resulting ForkedFrom
be declared? And would be it be okay to make the fetchForkedFromResult
method inside API+PackageController+GetRoute
return the new ForkedFrom
type and then pass it down to the Model through the initializer.
Could you please take a closer look when you find the time and propose the change, since I am not entirely sure what's the correct approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened a PR on top of your branch with the changes here: #3388
That should sort it all out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! The comments on the PR were really helpful! I've merged it into the branch.
@@ -40,4 +40,12 @@ extension Joined3 where M == Package, R1 == Repository, R2 == Version { | |||
.filter(Repository.self, \.$owner, .custom("ilike"), owner) | |||
.filter(Repository.self, \.$name, .custom("ilike"), repository) | |||
} | |||
|
|||
static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Dave, we shouldn't be filtering inside the method. If you look above, there's a version related overload and I'd do the same here. Keep the parameter, that way it's obvious at the call site that versions are filtered:
static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> { | |
static func query(on database: Database, | |
packageId: Package.Id, | |
version: Version.Kind) -> JoinedQueryBuilder<Self> { | |
query(on: database, version: version) | |
.filter(Package.self, \Package.$id == packageId) | |
} |
Call site:
let model = try await Joined3<Package, Repository, Version>
.query(on: database, packageId: packageId, version: .defaultBranch)
.first()
Sources/App/Models/Repository.swift
Outdated
static func find(on database: Database, for packageId: Package.Id) async throws -> Repository? { | ||
return try await Repository.query(on: database) | ||
.filter(\.$package.$id == packageId) | ||
.first() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be unused?
static func find(on database: Database, for packageId: Package.Id) async throws -> Repository? { | |
return try await Repository.query(on: database) | |
.filter(\.$package.$id == packageId) | |
.first() | |
} |
Merge ForkedFromResult and ForkedFromInfo
There's one other thing to consider: |
I've been investigating this a little bit this morning and this is what I found. Package deletion and renaming happens in the func reconcileLists(db: Database, source: [URL], target: [URL]) async throws {
let (toAdd, toDelete) = diff(source: source, target: target)
let insert = toAdd.map { Package(url: $0, processingStage: .reconciliation) }
try await insert.create(on: db)
for url in toDelete {
try await Package.query(on: db)
.filter(by: url)
.delete()
}
} and since What we could is before the deletion of a Package to check if any other Package has a As for the rename (delete + add), I am not sure how we can be certain that a package is being renamed in the |
I think this might be spreading this feature a little too far and wide and there may be a couple of things we can do to cope with this closer to the source of the feature.
That way, we never crash and any fork information should be updated within a few hours of the rename. |
I agree with this. Since we have this: extension API.PackageController.GetRoute.Model.ForkedFromInfo {
static func query(on database: Database, packageId: Package.Id) async -> Self? {
let model = try? await Joined3<Package, Repository, Version>
.query(on: database, packageId: packageId, version: .defaultBranch)
.first()
guard let repoName = model?.repository.name,
let ownerName = model?.repository.ownerName,
let owner = model?.repository.owner else {
return nil
}
return .fromSPI(originalOwner: owner,
originalOwnerName: ownerName,
originalRepo: repoName,
originalPackageName: model?.version.packageName ?? repoName)
}
} if the |
Thanks for the additional analysis, @supersonicbyte . That matches my expectations as well. Given that package removals / renames are rare (as are forks, at least at the moment), a window of a few hours where a fork isn't properly indicated doesn't strike me as a big problem. What we could also do, is extend the
Now even if the The fact that the url itself is also outdated isn't a big problem, because Github maintains redirects. I mention this for sake of completeness and for consideration. I'm leaning more towards just keeping it simple and letting it self-correct within a few hours myself. |
One final note: we could also at least still indicate that it's a fork even if we can't link to it temporarily. That wouldn't require any schema changes and still communicate the most important piece of info, that it's a derived package. |
That's actually a clever idea! Fallback to GitHub url and they handle the redirect. I think it should be simple to add. I can implement that, but you call the shots. |
We could do that by adding an |
If we're considering changing the enum, let's change it to also include the URL as a string so we can do the former suggestion from @finestructure rather than doing this. |
There are different enums though. |
Bumps the npm-dependencies group with 2 updates: [postcss](https://github.com/postcss/postcss) and [stylelint-scss](https://github.com/stylelint-scss/stylelint-scss). Updates `postcss` from 8.4.45 to 8.4.47 - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.4.45...8.4.47) Updates `stylelint-scss` from 6.5.1 to 6.6.0 - [Release notes](https://github.com/stylelint-scss/stylelint-scss/releases) - [Changelog](https://github.com/stylelint-scss/stylelint-scss/blob/master/CHANGELOG.md) - [Commits](stylelint-scss/stylelint-scss@v6.5.1...v6.6.0) --- updated-dependencies: - dependency-name: postcss dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-dependencies - dependency-name: stylelint-scss dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm-dependencies ... Signed-off-by: dependabot[bot] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really a great addition to the site @supersonicbyte. Thank you so much for all of your work on it and we're delighted to get it merged and live for everyone to use. 👍
Thank you
Adds the forked from feature on the UI. Waiting for @daveverwer to supply the icon for this feature so it can be added.
Current state is this: